Skip to content

JS: Ignore JSON-like files in js/useless-expression#3401

Merged
semmle-qlci merged 3 commits into
github:masterfrom
erik-krogh:jsonLike
May 6, 2020
Merged

JS: Ignore JSON-like files in js/useless-expression#3401
semmle-qlci merged 3 commits into
github:masterfrom
erik-krogh:jsonLike

Conversation

@erik-krogh

@erik-krogh erik-krogh commented May 4, 2020

Copy link
Copy Markdown
Contributor

The anomalous query results found a project with a lot of useless-expressions.

However, most of those results were from JSON-like files. And while the expressions are useless, the warning looks benign.

I added a condition on the js/useless-expression so it doesn't report an expression if the containing file only contains that expression.
That pattern is somewhat common.

I could possibly restrict it further, such that we still report a useless expression if the expression contains a function?

@erik-krogh erik-krogh added the JS label May 4, 2020
@erik-krogh erik-krogh requested a review from a team as a code owner May 4, 2020 11:07
@esbena

esbena commented May 4, 2020

Copy link
Copy Markdown
Contributor

If the goal is to exclude json-like constructs only, then I think we should try a little harder to weed out illegal json. Some ideas: can we remove variable references, function literals, regexp literals, calls?

(We have previously tried to classify entire json-like files as generated, but we never found a scalable way to to that reliably.)

@asgerf

asgerf commented May 4, 2020

Copy link
Copy Markdown
Contributor

Looking great! 👍

We should probably exclude CodeInAttribute top-levels from this check, otherwise LGTM.

@erik-krogh

Copy link
Copy Markdown
Contributor Author

Some ideas: can we remove variable references, function literals, regexp literals, calls?

I've removed function literals from the check for now. When I look at the results we no longer flag I only want to remove functions from those.
The rest with e.g. binary expressions look like a benign parser-tests or something.

We should probably exclude CodeInAttribute top-levels from this check, otherwise LGTM.

I don't think there is any need for that.
Code in an attribute is already not in a void-context, and thus something like <div onclick="[1,2,3]"></div> is already not flagged.

@erik-krogh

Copy link
Copy Markdown
Contributor Author

An evaluation looks fine.
Lots of parser tests are no longer flagged as useless.

@semmle-qlci semmle-qlci merged commit 9210660 into github:master May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants